Don't make tools responsible for checking unknown and renamed lints #80524

jyn514 · 2020-12-30T18:55:58Z

Previously, clippy (and any other tool emitting lints) had to have their
own separate UNKNOWN_LINTS pass, because the compiler assumed any tool
lint could be valid. Now, as long as any lint starting with the tool
prefix exists, the compiler will warn when an unknown lint is present.

This may interact with the unstable tool_lint feature, which I don't entirely understand, but it will take the burden off those external tools to add their own lint pass, which seems like a step in the right direction to me.

Don't mark ineffective_unstable_trait_impl as an internal lint
Use clippy's more advanced lint suggestions
Deprecate the UNKNOWN_CLIPPY_LINTS pass (and make it a no-op)
Say 'unknown lint clippy::x' instead of 'unknown lint x'

This is tested by existing clippy tests. When #80527 merges, it will also be tested in rustdoc tests. AFAIK there is no way to test this with rustc directly.

rust-highfive · 2020-12-30T18:56:01Z

r? @matthewjasper

(rust-highfive has picked a reviewer for you, use r? to override)

rust-log-analyzer · 2020-12-30T19:05:51Z

The job x86_64-gnu-llvm-9 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

   Compiling miniz_oxide v0.4.0
   Compiling object v0.22.0
   Compiling hashbrown v0.9.0
   Compiling addr2line v0.14.0
error: unknown lint: `rustc::existing_doc_keyword`
    |
    |
216 | #![deny(rustc::existing_doc_keyword)]
    |
    |
    = note: `-D unknown-lints` implied by `-D warnings`
error: aborting due to previous error

error: could not compile `std`

flip1995 · 2020-12-31T10:17:42Z

Something that is also important to mention:

tool_lints are semi-stable. So you can use them without adding a feature, but only explicitly allowed tools (defined in is_known_lint_tool can implement them. This also means, that this is a semi-breaking change. The reference says:

If a lint attribute, such as allow, references a nonexistent tool lint, the compiler will not warn about the nonexistent lint until you use the tool.

So it doesn't mention, that the handling of missing lints has to be implemented by the tool. With this and the fact, that we know all the tools that implement tool lints, this change should be fine.

flip1995 · 2020-12-31T11:12:14Z

So the CI error is because internal lints are only enabled in the Mode::Rustc, not when compiling std.

This makes the rustc::existing_doc_keywords lint useless, since it is only enabled in std. But enabling internal lints in std results in many warnings / allows of the rustc::default_hash_types lint.

flip1995 · 2020-12-31T11:33:55Z

Oh well, this uncovered a whole new set of problems, that were silently ignored before: rustc::internal lints are also tool lints, but rustc doesn't implement a unknown_rustc_lints lint. So writing deny(rustc::internal) where internal lints aren't enabled (std or even rustc_macros for some reason), was just silently ignored. This PR fixes that. But now when a tool like Clippy is compiled with ./x.py, which also uses internal rustc lints it fails, because they are not enabled by tools. They can be enabled by adding Mode::ToolsRustc here:

rust/src/bootstrap/builder.rs

Lines 1268 to 1271 in 8b002d5

    
           if mode == Mode::Rustc { 
        
               rustflags.arg("-Zunstable-options"); 
        
               rustflags.arg("-Wrustc::internal"); 
        
           }

But this results in build failures for other tools, that don't adhere to the rustc internal lints, like rustdoc.

Well, this is something for the new year. Have a great new years eve everyone 🎉

jyn514 · 2021-01-01T03:57:35Z

But enabling internal lints in std results in many warnings / allows of the rustc::default_hash_types lint.

FWIW, adding -Wrustc::internal -Arustc::default-hash-types gives no warnings. So in theory we could special case std like that. I think existing_doc_keywords is niche enough it shouldn't be a big deal to remove it though (cc @GuillaumeGomez just in case - I kind of wish you'd pinged me on #79541, since I suggested it in #79464 (comment)).

But now when a tool like Clippy is compiled with ./x.py, which also uses internal rustc lints it fails, because they are not enabled by tools.

I don't quite understand this - I think to test it I would need x.py build --stage 2, which takes a while. Can you give an example of a lint that fails?

But this results in build failures for other tools, that don't adhere to the rustc internal lints, like rustdoc.

I'm happy to fix the lints for rustdoc, I would prefer to have them enabled anyway.

GuillaumeGomez · 2021-01-01T11:18:01Z

existing_doc_keywords is aimed to rustc only, not rustdoc or anything else (which means you can still add doc on keywords that don't exist outside of rustc). The goal is to prevent rustc to have doc on keywords which don't exist while allowing other crates to (since it's aimed for proc-macros). So please don't remove it?

jyn514 · 2021-01-01T14:26:38Z

@GuillaumeGomez it's aimed at libstd, which is the whole point - your PR did nothing because rustc lints aren't applied to libstd currently. I'm ok with denying that lint specifically for std though.

GuillaumeGomez · 2021-01-01T14:31:03Z

libstd and libcore to be exact. If it's denied for both of them then it's all good for me.

flip1995 · 2021-01-02T14:33:02Z

I don't quite understand this - I think to test it I would need x.py build --stage 2, which takes a while. Can you give an example of a lint that fails?

So Clippy has #[deny(rustc::internal)] in the lib.rs. But since for Mode::ToolsRustc internal lints aren't enabled currently, the compilation will fail (./x.py test src/tools/clippy). Your changes in #80573 should fix this though.

The question then is if other tools want those internal lints.

matthewjasper

r=me with nit addressed (and CI passing)

compiler/rustc_lint/src/context.rs

jyn514 · 2021-01-15T20:11:02Z

I don't quite understand this - I think to test it I would need x.py build --stage 2, which takes a while. Can you give an example of a lint that fails?

So Clippy has #[deny(rustc::internal)] in the lib.rs. But since for Mode::ToolsRustc internal lints aren't enabled currently, the compilation will fail (./x.py test src/tools/clippy). Your changes in #80573 should fix this though.

The question then is if other tools want those internal lints.

So it turns out the issue is that rustc::ineffective_unstable_trait_impl is on unconditionally, because it's not actually an internal lint, it's a lint for staged_api. So even when internal lints are disabled, my new logic sees rustc:: and thinks all rustc:: lints are known.

FYI @m-ou-se I'm going to change this to just be ineffective_unstable_trait_impl, does that sound good? It will still be emitted only for staged_api.

There's a second question here which is why all rustc:: lints are not known when -W rustc::internal isn't passed, but I'm planning to leave that mystery to another day if possible.

jyn514 · 2021-01-15T20:15:15Z

#76538 (comment):

Added it to the builtin lints as a rustc:: tool lint, just like the internal lints. That way it doesn't show up in -W help and an #[allow()] attribute for it doesn't trigger a unknown lint error (which happens when bootstrapping rustc+std, using the current beta).

Normally the fix for this is to add cfg(not(bootstrap)) to the allow. I don't have a fix for it showing up in -W help, but since people besides the standard library can use staged_api on nightly, I'm ok with adding it there.

m-ou-se · 2021-01-15T20:37:24Z

@jyn514 Yeah, that lint is a bit of a weird one. It's mostly/only meant for std but still part of an unstable feature usable outside of it. Rustc doesn't really have a concept of "lint that's part of an unstable/private feature". Putting it under rustc:: was a hack. It seems fine to me to change that.

flip1995 · 2021-01-15T20:42:02Z

lints are not known when -W rustc::internal isn't passed, but I'm planning to leave that mystery to another day if possible.

Oh it isn't because of the -W flag, it's the -Zunstable-options flag, that gates the registration of internal lints. If internal lints were registered unconditionally, it could have a negative impact on performance.

We chose to (ab)use the unstable-options option for internal lints, because we didn't think it was worth to add an extra option for it (or smth like this).

Clippy sets this flag in .cargo/config.

jyn514 · 2021-01-15T20:51:42Z

Ahh, that makes sense.

So to summary, this does the following things:

Rename rustc::ineffective_unstable_trait_impl to ineffective_unstable_trait_impl.
Move checking of unknown and renamed lints to the lint machinery, so that each new tool doesn't have to add its own pass checking it. Assume that if any tool:: lint is present, all lints for tool are known. In particular this is necessary to avoid warning on #[deny(rustc::internal)] when -Z unstable-options isn't passed, and in general we have to assume at some point that the lints are known or we can't warn about unknown lints.
Fix 'unknown lint x' to say 'unknown lint tool::x' instead.

@flip1995 does all of that sound good? I think for 2 you'll want to deprecate/remove the clippy pass that currently checks for unknown lints - is that simple to do? Happy to do it myself if you let me know how.

flip1995 · 2021-01-15T21:00:37Z

Yes, that sounds good to me.

If everything in this PR works as expected, every error message in the test for unknown_clippy_lints should be duplicated. If that is the case, I will deprecate the Clippy lint for you (I can either post a patch file or you can give me push access to your fork, so I can push directly to this branch).

It's not an internal lint: - It's not in the rustc::internal lint group - It's on unconditionally, because it actually lints `staged_api`, not the compiler This fixes a bug where `#[deny(rustc::internal)]` would warn that `rustc::internal` was an unknown lint.

compiler/rustc_lint/src/context.rs

src/tools/clippy/clippy_lints/src/deprecated_lints.rs

src/tools/clippy/clippy_lints/src/lib.rs

jyn514 · 2021-01-16T17:14:47Z

src/tools/clippy/tests/ui/unknown_clippy_lints.stderr

Hmm, I'm curious to know why this changed. This is a bug fix, right, it should have given a warning before?

Hm? The order is just different. And it has the default weirdness with linting attributes, that it is duplicated for the first inner attribute.

Ah ok, I misread. No need to change anything.

And it has the default weirdness with linting attributes, that it is duplicated for the first inner attribute.

Does this have an issue open? It seems worth fixing.

I don't know of an issue for this.

I opened rust-lang/rust-clippy#6602.

I'm not sure if it's just in a different order; at the bottom it shows that it emitted 8 errors previously and now it emits 9.

This copies the unknown_lints code clippy uses for its unknown_clippy_lints lint to rustc. The unknown_clippy_lints code is more advanced, because it doesn't suggest renamed or removed lints and correctly suggest lower casing lints.

This is now handled by unknown_lints

jyn514 · 2021-01-17T15:28:46Z

Alright, I think this is good to go!

@bors r=flip1995,matthewjasper

bors · 2021-01-17T15:28:47Z

📌 Commit 13728b8 has been approved by flip1995,matthewjasper

bors · 2021-01-17T17:52:07Z

⌛ Testing commit 13728b8 with merge 1f0fc02...

bors · 2021-01-17T20:49:04Z

☀️ Test successful - checks-actions
Approved by: flip1995,matthewjasper
Pushing 1f0fc02 to master...

The clippy unknown lints attribute was deprecated in nightly in rust-lang/rust#80524. The old lint name now produces a warning. Since we're using `allow(unknown_lints)` to suppress warnings, we need to adopt the canonical name, so we can continue to build without warnings.

The clippy unknown lints attribute was deprecated in nightly in rust-lang/rust#80524. The old lint name now produces a warning. Since we're using `allow(unknown_lints)` to suppress warnings, we need to adopt the canonical name, so we can continue to build without warnings. This change was automatically generated using the following script: ```sh sd clippy::unknown_clippy_lints unknown_lints \ $(fd . --extension rs zebra*)` ```

The clippy unknown lints attribute was deprecated in nightly in rust-lang/rust#80524. The old lint name now produces a warning. Since we're using `allow(unknown_lints)` to suppress warnings, we need to add the canonical name, so we can continue to build without warnings on nightly. But we also need to keep the old name, so we can continue to build without warnings on stable. And therefore, we also need to disable the "removed lints" warning, otherwise we'll get warnings about the old name on nightly. We'll need to keep this transitional clippy config until rustc 1.51 is stable.

jyn514 added A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. labels Dec 30, 2020

rust-highfive assigned matthewjasper Dec 30, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 30, 2020

jyn514 mentioned this pull request Dec 30, 2020

Make rustdoc lints a tool lint instead of built-in #80527

Merged

jyn514 mentioned this pull request Jan 1, 2021

Deny rustc::internal lints for rustdoc and clippy #80573

Merged

matthewjasper reviewed Jan 10, 2021

View reviewed changes

compiler/rustc_lint/src/context.rs Outdated Show resolved Hide resolved

jyn514 mentioned this pull request Jan 12, 2021

Add x.py check --stage 1 #80952

Closed

jyn514 force-pushed the unknown-tool-lints branch 2 times, most recently from 4d7dcbe to 5db673c Compare January 15, 2021 20:28

This comment has been minimized.

Sign in to view

jyn514 force-pushed the unknown-tool-lints branch from 5db673c to 77b15c1 Compare January 15, 2021 20:45

This comment has been minimized.

Sign in to view

jyn514 assigned flip1995 Jan 15, 2021

This comment has been minimized.

Sign in to view

jyn514 force-pushed the unknown-tool-lints branch from f7a2a1f to f333a05 Compare January 15, 2021 21:59

This comment has been minimized.

Sign in to view

jyn514 force-pushed the unknown-tool-lints branch from f333a05 to c819a4c Compare January 15, 2021 22:31

jyn514 commented Jan 16, 2021

View reviewed changes

flip1995 added 2 commits January 16, 2021 19:44

More advanced unknown lint suggestion

5e3df42

This copies the unknown_lints code clippy uses for its unknown_clippy_lints lint to rustc. The unknown_clippy_lints code is more advanced, because it doesn't suggest renamed or removed lints and correctly suggest lower casing lints.

Deprecate unknown_clippy_lints

13728b8

This is now handled by unknown_lints

flip1995 force-pushed the unknown-tool-lints branch from 2e70434 to 13728b8 Compare January 16, 2021 18:45

jyn514 mentioned this pull request Jan 17, 2021

Clippy warns twice about unknown lints rust-lang/rust-clippy#6602

Closed

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 17, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 17, 2021

bors merged commit 1f0fc02 into rust-lang:master Jan 17, 2021

rustbot added this to the 1.51.0 milestone Jan 17, 2021

jyn514 deleted the unknown-tool-lints branch January 17, 2021 20:49

This was referenced Jan 17, 2021

Stabilize the Wake trait #74304

Merged

Store HIR attributes in a side table #79519

Merged

teor2345 mentioned this pull request Jan 18, 2021

Use the rustc unknown lints attribute ZcashFoundation/zebra#1609

Merged

Don't make tools responsible for checking unknown and renamed lints #80524

Don't make tools responsible for checking unknown and renamed lints #80524

Uh oh!

Conversation

jyn514 commented Dec 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Dec 30, 2020

Uh oh!

rust-log-analyzer commented Dec 30, 2020

Uh oh!

flip1995 commented Dec 31, 2020

Uh oh!

flip1995 commented Dec 31, 2020

Uh oh!

flip1995 commented Dec 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jyn514 commented Jan 1, 2021

Uh oh!

GuillaumeGomez commented Jan 1, 2021

Uh oh!

jyn514 commented Jan 1, 2021

Uh oh!

GuillaumeGomez commented Jan 1, 2021

Uh oh!

flip1995 commented Jan 2, 2021

Uh oh!

matthewjasper left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jyn514 commented Jan 15, 2021

Uh oh!

jyn514 commented Jan 15, 2021

Uh oh!

m-ou-se commented Jan 15, 2021

Uh oh!

flip1995 commented Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

jyn514 commented Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flip1995 commented Jan 15, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

jyn514 Jan 16, 2021

Choose a reason for hiding this comment

Uh oh!

flip1995 Jan 16, 2021

Choose a reason for hiding this comment

Uh oh!

jyn514 Jan 16, 2021

Choose a reason for hiding this comment

Uh oh!

flip1995 Jan 16, 2021

Choose a reason for hiding this comment

Uh oh!

jyn514 Jan 17, 2021

Choose a reason for hiding this comment

Uh oh!

camelid Jan 17, 2021

Choose a reason for hiding this comment

Uh oh!

jyn514 commented Jan 17, 2021

Uh oh!

bors commented Jan 17, 2021

Uh oh!

bors commented Jan 17, 2021

Uh oh!

bors commented Jan 17, 2021

Uh oh!

jyn514 commented Dec 30, 2020 •

edited

Loading

flip1995 commented Dec 31, 2020 •

edited

Loading

matthewjasper left a comment •

edited

Loading

flip1995 commented Jan 15, 2021 •

edited

Loading

jyn514 commented Jan 15, 2021 •

edited

Loading